backend/app/crud/evaluations/embeddings.py (1)

366-367: Misleading comment - update to reflect actual behavior.

The comment says "Get embedding model from config" but the code hardcodes the value. Update the comment to accurately describe the implementation.

-        # Get embedding model from config (default: text-embedding-3-large)
-        embedding_model = "text-embedding-3-large"
+        # Use fixed embedding model (text-embedding-3-large)
+        embedding_model = "text-embedding-3-large"

backend/app/tests/api/routes/test_evaluation.py (1)

524-545: Consider renaming function to match its new purpose.

The function test_start_batch_evaluation_missing_model was repurposed to test invalid config_id scenarios. The docstring was updated but the function name still references "missing_model". Consider renaming for clarity.

-    def test_start_batch_evaluation_missing_model(self, client, user_api_key_header):
-        """Test batch evaluation fails with invalid config_id."""
+    def test_start_batch_evaluation_invalid_config_id(self, client, user_api_key_header):
+        """Test batch evaluation fails with invalid config_id."""

backend/app/api/routes/evaluation.py (1)

499-510: Consider validating that model is present in config params.

The model is extracted with .get("model") which returns None if not present. Since model is critical for cost tracking (used in create_langfuse_dataset_run), consider validating its presence and returning an error if missing.

     # Extract model from config for storage
     model = config.completion.params.get("model")
+    if not model:
+        raise HTTPException(
+            status_code=400,
+            detail="Config must specify a 'model' in completion params for evaluation",
+        )

     # Create EvaluationRun record with config references

backend/app/crud/evaluations/core.py (1)

15-69: Config-based create_evaluation_run refactor is correctly implemented; consider logging model for improved traceability.

The refactor from inline config dict to config_id: UUID and config_version: int is properly implemented throughout:

• The sole call site in backend/app/api/routes/evaluation.py:503 correctly passes all new parameters with the right types (config_id as UUID, config_version as int, model extracted from config).
• The EvaluationRun model in backend/app/models/evaluation.py correctly defines all three fields with appropriate types and descriptions.
• All type hints align with Python 3.11+ guidelines.

One suggested improvement for debugging:

Include model in the creation log for better traceability when correlating evaluation runs with model versions:

logger.info(
    f"Created EvaluationRun record: id={eval_run.id}, run_name={run_name}, "
-   f"config_id={config_id}, config_version={config_version}"
+   f"config_id={config_id}, config_version={config_version}, model={model}"
)

Since the model is already extracted at the call site and passed to the function, including it in the log will provide fuller context for operational debugging without any additional cost.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

backend/app/models/evaluation.py (1)

148-158: Align EvaluationRun type hints with nullable DB columns for config fields

config_id and config_version are nullable in the schema but annotated as non-optional types. This can mislead callers and type checkers into assuming they’re always present, even for legacy runs or transitional data.

Consider updating the annotations to reflect nullability:

-    config_id: UUID = SQLField(
+    config_id: UUID | None = SQLField(
         foreign_key="config.id",
         nullable=True,
         description="Reference to the stored config used for this evaluation",
     )
-    config_version: int = SQLField(
+    config_version: int | None = SQLField(
         nullable=True,
         ge=1,
         description="Version of the config used for this evaluation",
     )

This keeps the schema the same while making runtime and type expectations clearer.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

backend/app/services/llm/providers/registry.py (1)

16-24: Handle the new OPENAI alias in get_llm_provider.

Line 16-24 registers "openai" but Line 66-74 only accepts OPENAI_NATIVE, so provider_type="openai" will raise ValueError despite being registered. This makes the new provider unusable.

🔧 Proposed fix

-    if provider_type == LLMProvider.OPENAI_NATIVE:
+    if provider_type in (LLMProvider.OPENAI_NATIVE, LLMProvider.OPENAI):
         if "api_key" not in credentials:
             raise ValueError("OpenAI credentials not configured for this project.")
         client = OpenAI(api_key=credentials["api_key"])

Also applies to: 66-74

backend/app/crud/evaluations/batch.py (1)

107-119: Avoid sending None fields in the Responses API payload.

Line 107-119 always includes optional fields and always builds tools, so vector_store_ids=None is sent when no KB is configured. This can cause request failures. Build the body conditionally and omit None values.

🔧 Proposed fix (conditional body)

-        batch_request = {
-            "custom_id": item["id"],
-            "method": "POST",
-            "url": "/v1/responses",
-            "body": {
-                # Use config as-is
-                "model": config.model,
-                "instructions": config.instructions,
-                "temperature": config.temperature,
-                "reasoning": {"effort": config.reasoning} if config.reasoning else None,
-                "tools": [
-                    {
-                        "type": "file_search",
-                        "vector_store_ids": config.knowledge_base_ids,
-                        "max_num_results": config.max_num_results or 20,
-                    }
-                ],
-                "input": question,  # Add input from dataset
-            },
-        }
+        body: dict[str, Any] = {"model": config.model, "input": question}
+        if config.instructions is not None:
+            body["instructions"] = config.instructions
+        if config.temperature is not None:
+            body["temperature"] = config.temperature
+        if config.reasoning is not None:
+            body["reasoning"] = {"effort": config.reasoning}
+        if config.knowledge_base_ids:
+            body["tools"] = [
+                {
+                    "type": "file_search",
+                    "vector_store_ids": config.knowledge_base_ids,
+                    "max_num_results": config.max_num_results or 20,
+                }
+            ]
+
+        batch_request = {
+            "custom_id": item["id"],
+            "method": "POST",
+            "url": "/v1/responses",
+            "body": body,
+        }

OpenAI Responses API: are optional fields allowed to be null, and must tools.vector_store_ids be a non-null list?

backend/app/crud/evaluations/core.py (1)

343-366: Validate completion params and model before returning.

Line 366 dereferences config.completion.params.model without checking if completion/params exist or if model is empty, which can raise or violate the declared str return contract.

🔧 Proposed fix

-    model = config.completion.params.model
-    return model
+    if not config.completion or not config.completion.params:
+        raise ValueError(
+            f"Config for evaluation {eval_run.id} is missing completion params "
+            f"(config_id={eval_run.config_id}, version={eval_run.config_version})"
+        )
+
+    model = config.completion.params.model
+    if not model:
+        raise ValueError(
+            f"Config for evaluation {eval_run.id} has no model specified "
+            f"(config_id={eval_run.config_id}, version={eval_run.config_version})"
+        )
+    return model

backend/app/services/evaluations/evaluation.py (1)

101-114: Add explicit None check for type safety.

After resolve_config_blob returns, the type of config is ConfigBlob | None. While the function contract implies config is non-None when error is None, accessing config.completion.provider on line 110 without an explicit check could cause issues with type checkers and is less defensive.

🛠️ Suggested improvement

     config, error = resolve_config_blob(
         config_crud=config_version_crud,
         config=LLMCallConfig(id=config_id, version=config_version),
     )
-    if error:
+    if error or config is None:
         raise HTTPException(
             status_code=400,
             detail=f"Failed to resolve config from stored config: {error}",
         )
-    elif config.completion.provider != LLMProvider.OPENAI:
+
+    if config.completion.provider != LLMProvider.OPENAI:
         raise HTTPException(
             status_code=422,
             detail="Only 'openai' provider is supported for evaluation configs",
         )

backend/app/tests/api/routes/test_evaluation.py (1)

554-577: Test name is inconsistent with test behavior after refactor.

The test method is named test_start_batch_evaluation_missing_model but the docstring and actual test behavior validate that the evaluation fails with an invalid/non-existent config_id. Consider renaming for clarity.

🛠️ Suggested rename

-    def test_start_batch_evaluation_missing_model(
+    def test_start_batch_evaluation_invalid_config_id(
         self, client: TestClient, user_api_key_header: dict[str, str]
     ) -> None:
         """Test batch evaluation fails with invalid config_id."""

backend/app/tests/api/routes/test_evaluation.py (1)

555-578: Consider renaming the test for clarity.

The test now validates invalid config_id, but the name still says “missing_model”. Renaming improves intent clarity.

♻️ Suggested rename

-def test_start_batch_evaluation_missing_model(
+def test_start_batch_evaluation_invalid_config_id(
     self, client: TestClient, user_api_key_header: dict[str, str]
 ) -> None:

backend/app/crud/evaluations/embeddings.py (1)

24-26: Consider adding an explicit type annotation to the constant.

Per the coding guidelines recommending type hints throughout the codebase, adding an explicit type annotation improves clarity and tooling support.

Suggested change

 # Default embedding model
-EMBEDDING_MODEL = "text-embedding-3-large"
+EMBEDDING_MODEL: str = "text-embedding-3-large"

backend/app/crud/evaluations/core.py (1)

372-392: Validate completion params/model before returning.

config.completion.params.model can be missing or None, which violates the declared str return type and can break downstream calls. This mirrors a previously flagged issue.

🔧 Proposed fix

-    if not eval_run.config_id or not eval_run.config_version:
+    if eval_run.config_id is None or eval_run.config_version is None:
         raise ValueError(
             f"Evaluation run {eval_run.id} has no config reference "
             f"(config_id={eval_run.config_id}, config_version={eval_run.config_version})"
         )
@@
-    model = config.completion.params.model
-    return model
+    if not config.completion or not config.completion.params:
+        raise ValueError(
+            f"Config for evaluation {eval_run.id} is missing completion params"
+        )
+
+    model = config.completion.params.model
+    if not model:
+        raise ValueError(
+            f"Config for evaluation {eval_run.id} does not specify a model "
+            f"(config_id={eval_run.config_id}, version={eval_run.config_version})"
+        )
+    return model

backend/app/tests/api/routes/test_evaluation.py (1)

529-544: Prefer deriving config_version from the created config.

Hardcoding config_version=1 (e.g., Line 543, Line 767, Line 807, Line 868, Line 906) can drift if create_test_config changes its default versioning. Consider using the version exposed by the created config object to keep tests resilient.

Also applies to: 759-768, 799-808, 862-868, 899-906

backend/app/alembic/versions/041_add_config_in_evals_run_table.py (2)

19-41: Prevent data loss when dropping evaluation_run.config.

Line 41 drops the old JSONB config without migrating existing data into config_id/config_version, which will permanently lose configuration data for existing rows. Please add a data migration/backfill (or explicitly document why data loss is acceptable) before dropping the column.

---

45-53: Downgrade will fail if rows exist due to non-nullable config.

op.add_column(..., nullable=False) will fail on a non-empty table. Make it nullable (or add a default/backfill before enforcing NOT NULL).

✅ Suggested fix

     op.add_column(
         "evaluation_run",
         sa.Column(
             "config",
             postgresql.JSONB(astext_type=sa.Text()),
             autoincrement=False,
-            nullable=False,
+            nullable=True,
             comment="Evaluation configuration (model, instructions, etc.)",
         ),
     )

backend/app/crud/evaluations/batch.py (2)

117-117: Redundant length check.

The expression config.knowledge_base_ids and len(config.knowledge_base_ids) > 0 is redundant. A non-empty list is truthy, so if config.knowledge_base_ids: suffices.

♻️ Proposed simplification

-        if config.knowledge_base_ids and len(config.knowledge_base_ids) > 0:
+        if config.knowledge_base_ids:

---

154-156: Update docstring to reflect the new type.

The docstring describes config as a "dict with llm, instructions, vector_store_ids" but it is now KaapiLLMParams with fields model, instructions, knowledge_base_ids, etc.

📝 Proposed docstring update

-        config: Evaluation configuration dict with llm, instructions, vector_store_ids
+        config: KaapiLLMParams with model, instructions, knowledge_base_ids, etc.